Fix DShot output for reversible motors when disarmed by sensei-hacker · Pull Request #11847 · iNavFlight/inav

sensei-hacker · 2026-09-01T04:37:25Z

Summary

Fixes writeMotors() so DShot output is correct for 3D/reversible motors during motor-testing while the FC is disarmed (the MSP_SET_MOTOR path used by the configurator's motor-test UI). Firmware companion to configurator PR #2595.

Problem

writeMotors() picked its DShot scaling formula from reversibleMotorsThrottleState/throttleRangeMin/throttleRangeMax, but those are only updated by mixTable()'s armed-flight direction-switching logic, which never runs while disarmed (mixTable() early-returns before reaching it). Consequences while disarmed:

reversibleMotorsThrottleState stays FORWARD, so motor values that should produce reverse thrust were instead sent DSHOT_DISARM_COMMAND.
Even forward values were checked against the RC-stick deadband (throttleRangeMin, ~1550) instead of the ESC's configured 3D deadband (reversibleMotorsConfig()->deadband_high, ~1514).

This matches field-reported behavior on a SpeedyBee F7 V3 (comment on #2595, 2026-03-19): motor testing works forward-only with reversible motors enabled.

Changes

src/main/flight/mixer.c: added an if (!ARMING_FLAG(ARMED)) branch inside writeMotors()'s FEATURE_REVERSIBLE_MOTORS DShot dispatch. While disarmed, direction is inferred directly from the motor value against reversibleMotorsConfig()->deadband_high/deadband_low, scaled into the DShot ranges. Armed-flight dispatch is completely unchanged.
src/test/unit/mixer_unittest.cc (new): unit tests covering forward/reverse/deadband scaling while disarmed, boundary values at exactly deadband_high/deadband_low, and regression coverage for armed flight and non-reversible motor testing. Includes "SourceSync" tests that read the live mixer.c at test time to guard against silent drift (the code under test can't be linked directly into this host test harness, since writeMotors() is #if !defined(SITL_BUILD)-gated and the harness always defines SITL_BUILD).

Testing

Unit tests: make check — 57/57 passing (new mixer_unittest: 16/16).
Built SITL clean, no warnings.
Built MATEKF405 with -DWARNINGS_AS_ERRORS=ON (matches CI recipe) — clean, Flash 68.14%, RAM 82.40%.
Hardware testing with a physical reversible ESC has NOT been done yet. Requesting a tester confirm both forward and reverse spin on real hardware before merge.

Code Review

Reviewed with inav-code-review agent — no critical issues; addressed the review's findings in the test file (removed an unverifiable precedent citation, fixed a boundary-condition mismatch between the test's oracle helpers and the real code's strict comparisons, added explicit boundary tests, trimmed changelog-style comments).

writeMotors() picked its DShot scaling formula from reversibleMotorsThrottleState/throttleRangeMin/throttleRangeMax, but those are only updated by mixTable()'s armed-flight direction-switching logic, which never runs while disarmed. As a result, 3D/reversible motor testing via MSP_SET_MOTOR (the configurator's motor-test UI) never produced reverse thrust, and even forward values were checked against the RC-stick deadband instead of the ESC's configured 3D deadband. While disarmed, infer direction directly from the motor value against reversibleMotorsConfig()->deadband_low/deadband_high instead. Armed-flight behavior is unchanged. Companion to inav-configurator PR iNavFlight#2595.

qodo-code-review · 2026-09-01T04:37:31Z

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

qodo-free-for-open-source-projects · 2026-09-01T04:38:56Z

PR Summary by Qodo

Fix disarmed DShot output for reversible motors

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Correct reversible DShot motor testing while disarmed using configured ESC deadbands.
• Preserve armed reversible and standard DShot scaling behavior.
• Add boundary, regression, and source-synchronization tests for mixer output scaling.

Diagram

graph TD
  A["Motor Test UI"] --> B["MSP_SET_MOTOR"] --> C["writeMotors"] --> D{"Armed?"}
  D -- Yes --> E["Existing Scaling"]
  D -- No --> F{"3D Range?"}
  F -- Forward --> G["DShot Thrust"]
  F -- Reverse --> G
  F -- Deadband --> H["Disarm Command"]

High-Level Assessment

The following are alternative approaches to this PR:

1. Extract a testable DShot scaling helper

➕ Tests the production implementation directly
➕ Eliminates duplicated scaling logic and source-text assertions
➕ Reduces maintenance when mixer formatting or surrounding code changes
➖ Broadens the production refactor beyond the immediate bug fix
➖ May require changing SITL guards and unit-test link dependencies

2. Update mixer direction state while disarmed

➕ Reuses the existing armed scaling dispatch
➕ Centralizes direction state for all output protocols
➖ Couples motor testing to armed-flight state machinery
➖ Risks altering mixTable early-return and flight behavior
➖ Still requires distinguishing RC deadbands from configured ESC deadbands

Recommendation: Keep the scoped writeMotors() production fix because it derives disarmed direction from the authoritative motor value and ESC deadbands without changing armed-flight behavior. If test infrastructure changes are acceptable, extracting the reversible DShot conversion into a small linkable helper would be preferable to maintaining a hand-copied implementation and brittle source-synchronization checks.

Files changed (2) +859 / -1

Bug fix (1) +19 / -1

mixer.cScale disarmed reversible DShot commands from ESC deadbands +19/-1

Scale disarmed reversible DShot commands from ESC deadbands

• Adds a disarmed path for reversible digital motors that infers forward, reverse, or neutral directly from each motor value and the configured 3D deadband. Forward and reverse values are scaled into their respective DShot ranges, while deadband values emit the disarm command; armed behavior remains unchanged.

src/main/flight/mixer.c

Tests (1) +840 / -0

mixer_unittest.ccCover reversible DShot scaling and regression paths +840/-0

Cover reversible DShot scaling and regression paths

• Adds unit coverage for disarmed forward, reverse, neutral, and exact deadband-boundary behavior, plus armed reversible and non-reversible regressions. Because the production path is excluded from SITL unit builds, the test mirrors the scaling logic and verifies synchronization against live mixer, header, and math sources.

src/test/unit/mixer_unittest.cc

qodo-free-for-open-source-projects · 2026-09-01T04:40:50Z

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. ~~Unsigned cast defeats clamping~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

The new branches assign scaleRangef() directly to the unsigned motorValue before calling
constrain(), so an input outside the configured source range is converted before it can be clamped
and can produce undefined or wrapped output. For example, the CLI-valid value 1000 with the valid
configuration min_command=1200, 3d_deadband_low=1406 scales negative in the reverse branch instead
of safely producing DSHOT_MIN_THROTTLE, potentially commanding unexpectedly high reverse thrust
during motor testing.

Code

src/main/flight/mixer.c[R389-392]

+                        motorValue = scaleRangef(motor[i],
+                            motorConfig()->mincommand, reversibleMotorsConfig()->deadband_low,
+                            DSHOT_MIN_THROTTLE, DSHOT_3D_DEADBAND_LOW);
+                        motorValue = constrain(motorValue, DSHOT_MIN_THROTTLE, DSHOT_3D_DEADBAND_LOW);

Evidence
motorValue is declared uint16_t, and both new branches assign the floating-point result before
constraining it. Configuration permits mincommand above the CLI's accepted lower motor-test value,
while the existing scaling helper deliberately uses a signed temporary and clamps before returning.
src/main/flight/mixer.c[268-290]
src/main/flight/mixer.c[372-392]
src/main/fc/settings.yaml[837-842]
src/main/fc/cli.c[3704-3710]
src/main/common/maths.c[228-232]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Disarmed reversible DShot scaling converts an out-of-range float to `uint16_t` before clamping, making the resulting motor command undefined or wrapped.
## Issue Context
`motorValue` is unsigned, while valid CLI motor-test inputs can be below a configured `mincommand`. The existing `handleOutputScaling()` avoids this ordering problem by retaining a signed temporary through the clamp.
## Fix Focus Areas
- src/main/flight/mixer.c[383-392]
- src/main/flight/mixer.c[268-290]
- src/main/fc/cli.c[3704-3710]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Equal endpoints divide by zero~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

The reverse mapping calls scaleRangef() with mincommand and deadband_low as source endpoints
even though the settings allow these values to be equal, yielding a zero denominator whenever a
lower MSP motor-test value reaches this branch. constrain() runs only after the invalid
floating-point conversion, so it cannot make the resulting DShot command reliable.

Code

src/main/flight/mixer.c[R388-391]

+                    } else if (motor[i] < reversibleMotorsConfig()->deadband_low) {
+                        motorValue = scaleRangef(motor[i],
+                            motorConfig()->mincommand, reversibleMotorsConfig()->deadband_low,
+                            DSHOT_MIN_THROTTLE, DSHOT_3D_DEADBAND_LOW);

Evidence
The new call uses a denominator of deadband_low - mincommand; both settings independently permit
1000, scaleRangef() performs an unchecked division by that difference, and MSP can supply a value
below the equal threshold to enter the branch.
src/main/flight/mixer.c[388-392]
src/main/common/maths.c[228-232]
src/main/fc/settings.yaml[837-842]
src/main/fc/settings.yaml[1304-1315]
src/main/fc/fc_msp.c[2320-2327]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Valid settings can make the new reverse scaling source range zero-width, causing division by zero and an invalid motor output.
## Issue Context
Both `min_command` and `3d_deadband_low` may be 1000, and `MSP_SET_MOTOR` accepts values below that threshold without range validation. Similar ordering problems should be handled defensively for the forward range as well.
## Fix Focus Areas
- src/main/flight/mixer.c[383-392]
- src/main/fc/settings.yaml[837-842]
- src/main/fc/settings.yaml[1304-1315]
- src/main/fc/fc_msp.c[2320-2327]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. ~~Boundary commands are disarmed~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

The strict >/< checks turn values exactly equal to deadband_high or deadband_low into
DSHOT_DISARM_COMMAND, whereas the existing armed reversible path treats those same boundary values
as directional commands and scales them to the minimum forward/reverse DShot values. Motor testing
therefore disagrees with flight behavior at both configured thrust boundaries.

Code

src/main/flight/mixer.c[R383-388]

+                    if (motor[i] > reversibleMotorsConfig()->deadband_high) {
+                        motorValue = scaleRangef(motor[i],
+                            reversibleMotorsConfig()->deadband_high, getMaxThrottle(),
+                            DSHOT_3D_DEADBAND_HIGH, DSHOT_MAX_THROTTLE);
+                        motorValue = constrain(motorValue, DSHOT_3D_DEADBAND_HIGH, DSHOT_MAX_THROTTLE);
+                    } else if (motor[i] < reversibleMotorsConfig()->deadband_low) {

Evidence
The new disarmed branch excludes equality, while armed direction selection uses >=/<= and the
armed output helper scales equality because its stop comparisons are strict in the opposite
direction. The throttle-status code likewise defines only the strict interior as deadband.
src/main/flight/mixer.c[280-287]
src/main/flight/mixer.c[383-395]
src/main/flight/mixer.c[576-591]
src/main/fc/rc_controls.c[114-126]
src/test/unit/mixer_unittest.cc[389-430]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Disarmed motor testing disarms exact deadband-boundary values even though armed reversible output treats them as minimum directional throttle.
## Issue Context
Direction selection in `mixTable()` uses inclusive comparisons, and `handleOutputScaling()` only stops values strictly beyond the active direction's threshold.
## Fix Focus Areas
- src/main/flight/mixer.c[383-394]
- src/main/flight/mixer.c[280-287]
- src/main/flight/mixer.c[576-591]
- src/test/unit/mixer_unittest.cc[389-430]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

4. Tests execute copied implementation 🐞 Bug ⚙ Maintainability

Description

All behavioral assertions call hand-written _CurrentSource copies rather than production
writeMotors() or handleOutputScaling(), while SourceSync only searches for selected source
substrings. The suite can therefore pass despite defects in surrounding dispatch, types,
conversions, or control flow and currently duplicates the unsigned-conversion and boundary bugs
instead of exposing them.

Code

src/test/unit/mixer_unittest.cc[R42-45]

+ * Why this file hand-reproduces mixer.c's logic instead of linking it
+ * -------------------------------------------------------------------
+ * writeMotors()'s entire body (and its helper handleOutputScaling()) is
+ * wrapped in `#if !defined(SITL_BUILD)`. Every unit test in this directory

Evidence
The test file defines local copies and every behavior test invokes those copies; production
implementations are excluded under SITL_BUILD, and the only linkage to live code is normalized
substring matching.
src/test/unit/mixer_unittest.cc[42-62]
src/test/unit/mixer_unittest.cc[122-235]
src/test/unit/mixer_unittest.cc[317-328]
src/test/unit/mixer_unittest.cc[681-765]
src/main/flight/mixer.c[267-292]
src/main/flight/mixer.c[369-372]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new tests validate a copied implementation rather than executable production code, allowing both copies to share the same defects.
## Issue Context
Refactor the small scaling/dispatch logic into a linkable helper or provide a focused build seam so unit tests invoke production code. Source text matching should not substitute for behavioral execution.
## Fix Focus Areas
- src/test/unit/mixer_unittest.cc[42-62]
- src/test/unit/mixer_unittest.cc[122-235]
- src/test/unit/mixer_unittest.cc[305-519]
- src/main/flight/mixer.c[267-292]
- src/main/flight/mixer.c[369-432]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Tip of the day

💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

github-actions · 2026-09-01T04:53:15Z

Test firmware build ready — commit b53eac8

Download firmware for PR #11847

245 targets built. Find your board's .hex file by name on that page (e.g. MATEKF405SE.hex). Files are individually downloadable — no GitHub login required.

Development build for testing only. Use Full Chip Erase when flashing.

github-actions · 2026-09-01T04:53:16Z

RAM / Flash usage vs. base branch — commit b53eac8

No size baseline is available yet for this PR's base commit (no per-commit baseline has been published for it). This comment will show deltas once one exists — rebasing the PR refreshes its base commit.

Target	Flash Δ	RAM Δ
MATEKF405	625191 B (no baseline)	133256 B (no baseline)
MATEKF722	468563 B (no baseline)	123312 B (no baseline)
MATEKF765	646099 B (no baseline)	138768 B (no baseline)
MATEKH743	664363 B (no baseline)	139776 B (no baseline)

See RAM/flash optimization guide for techniques to reduce usage.

The disarmed-motor-testing scaling assigned scaleRangef()'s float result directly into an unsigned motorValue before constrain() ran (undefined behavior for negative results, reachable since min_command/MSP_SET_MOTOR allow it), could divide by zero when min_command/3d_deadband_low or 3d_deadband_high/max_throttle are configured equal, and used exclusive boundary comparisons where the rest of the codebase (mixTable's direction switch, handleOutputScaling's stop check) treats boundary values as thrust, not deadband. Extracted the calculation into calculateDisarmedReversibleMotorsDshotValue(), using inclusive boundaries, explicit degenerate-config guards before scaling, and a signed intermediate before constrain(). Not SITL_BUILD-gated (pure arithmetic), so it stays isolated from the surrounding hardware-only code.

sensei-hacker · 2026-09-01T05:16:34Z

Addressed the 4 Qodo findings (all verified reachable via valid CLI configuration, not just theoretical):

Unsigned-cast-before-clamp / undefined behavior: the disarmed scaling assigned scaleRangef()'s float result directly into uint16_t motorValue before constrain() ran. Fixed by using a signed int32_t intermediate, matching constrain()'s actual signature.
Division by zero: min_command/3d_deadband_low (and 3d_deadband_high/max throttle) can be configured equal via the CLI, zeroing scaleRangef()'s denominator. Added explicit degenerate-config guards that return the boundary DShot constant directly instead of dividing.
Boundary values incorrectly disarmed: the disarmed branch used strict >/<, but the rest of the codebase (mixTable()'s direction switch, handleOutputScaling()'s stop check) treats a value exactly at the deadband boundary as thrust, not deadband. Switched to inclusive >=/<= to match.
Tests only exercised a hand-copy: extracted the disarmed calculation into its own small helper, calculateDisarmedReversibleMotorsDshotValue(), which is not SITL_BUILD-gated (pure arithmetic, no hardware dependency) — unlike writeMotors() itself. The unit test still can't link it directly (it's static, and linking mixer.c wholesale needs stubbing several globals), but the reproduction now targets one small, flat, easily-verified function instead of interleaved dispatch logic, with new regression tests for both boundary values and both degenerate-config guards.

Unit tests: 60/60 passing. Rebuilt SITL and MATEKF405 (-DWARNINGS_AS_ERRORS=ON) clean after these changes.

The unit test for the disarmed reversible-motor DShot scaling only ever called a hand-reproduction of the logic, since writeMotors() is SITL_BUILD-gated and can't be linked into the host test harness. A hand-copy can't catch a regression introduced directly in mixer.c. Extracted calculateDisarmedReversibleMotorsDshotValue() into its own file (mixer_disarmed_dshot.c/.h), with no INAV-specific dependencies beyond common/maths.c, and linked it into mixer_unittest.cc via CMakeLists.txt's existing `depends` mechanism (already used for telemetry/hott.c, io/rcdevice.c, etc.). The test now calls the real function directly. DSHOT_* constants moved to their own mixer_dshot_constants.h so the new file doesn't need to pull in mixer.h's drivers/timer.h dependency.

sensei-hacker · 2026-09-02T00:29:22Z

Follow-up: fully addressed Qodo finding #4 ("tests execute copied implementation").

calculateDisarmedReversibleMotorsDshotValue() is now in its own file (mixer_disarmed_dshot.c/.h), with no INAV-specific dependencies beyond common/maths.c (no motorConfig(), feature(), ARMING_FLAG(), etc.). mixer_unittest.cc links this real file directly via CMakeLists' existing depends mechanism (already used for telemetry/hott.c, io/rcdevice.c, etc. elsewhere in this test suite) and calls the real function — no more hand-copy for this logic. DSHOT_* constants moved into their own mixer_dshot_constants.h so the new file doesn't pull in mixer.h's drivers/timer.h dependency (which isn't buildable in the host test environment).

Verified this actually catches regressions: reverted the boundary comparisons in the real file to the old buggy >/<, rebuilt, confirmed exactly the 4 boundary/degenerate-config tests failed (13 others still passed), then restored and reverified clean.

Unit tests: 58/58 passing. SITL and MATEKF405 (-DWARNINGS_AS_ERRORS=ON) both rebuilt clean — identical flash/RAM (68.14%/82.40%), confirming the extraction is behavior- and size-neutral.

sensei-hacker · 2026-09-02T03:54:12Z

@Pikkuboo I don't suppose you could test this with iNavFlight/inav-configurator#2595 ?

sensei-hacker added this to the 9.1 milestone Sep 1, 2026

qodo-free-for-open-source-projects Bot reviewed Sep 1, 2026

View reviewed changes

Comment thread src/main/flight/mixer.c Outdated

Comment thread src/main/flight/mixer.c Outdated

Comment thread src/main/flight/mixer.c Outdated

Comment thread src/test/unit/mixer_unittest.cc Outdated

sensei-hacker added the Testing Required label Sep 1, 2026

sensei-hacker mentioned this pull request Sep 2, 2026

Allow motor test mode when reversible motors (3D) enabled iNavFlight/inav-configurator#2595

Open

Navigation Menu

Uh oh!

Fix DShot output for reversible motors when disarmed - #11847

Fix DShot output for reversible motors when disarmed#11847
sensei-hacker wants to merge 3 commits into
iNavFlight:release/9.1from
sensei-hacker:fix-3d-dshot-motor-testing-disarmed

sensei-hacker commented Sep 1, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Sep 1, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 1, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 1, 2026 •

edited

Loading

Uh oh!

sensei-hacker commented Sep 1, 2026

Uh oh!

sensei-hacker commented Sep 2, 2026 •

edited

Loading

Uh oh!

sensei-hacker commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

sensei-hacker commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Changes

Testing

Code Review

Related

Uh oh!

qodo-code-review Bot commented Sep 1, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 1, 2026

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sensei-hacker commented Sep 1, 2026

Uh oh!

sensei-hacker commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sensei-hacker commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

sensei-hacker commented Sep 1, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Sep 1, 2026 •

edited

Loading

github-actions Bot commented Sep 1, 2026 •

edited

Loading

github-actions Bot commented Sep 1, 2026 •

edited

Loading

sensei-hacker commented Sep 2, 2026 •

edited

Loading